Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destruct CowData more graciously by avoiding accidentally exposing a half-destructed buffer. #100694

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 21, 2024

This can avoid problems with cyclic references (and breakers).
You may think it's rare, but one such cyclic reference is currently occurring in GDScript compiler behavior and leading to half-destructed buffers being copied around. This is not leading to problems right now, because of a coincidental inefficiency in CowData is leading to a technically unnecessary fork of the data before double destruction can occur. In the worst case, the array would have been reallocated halfway through destruction, causing undefined behavior and possibly segmentation faults.

My PR removing the unnecessary forks (#100619) triggered the bug more harshly, and this led to very strange leaks occurring.

Importantly, master currently fails this simple sanity check:

75b7a03

This PR no longer fails the sanity check.

I also added a test to make sure there is no regression of this behavior.

@hpvb
Copy link
Member

hpvb commented Dec 21, 2024

Very impressive work! That took quite some sleuthing.

I wonder, however, of instead we should delete operator& from all of our CoWing types (Vector, String, etc), reason being that by taking a pointer to the container you circumvent CoW, which is what, in my mind, lies at the root of this.

I think with a CoW datatype taking a pointer to it should be an error, because you should never be storing a reference to the container, you should always take a copy and rely on CoW to make it safe.

If a piece of code can't do that, then it shouldn't be using CoW to begin with. Especially now with the move operators, I feel that if you need to do processing on a CoW type to later pass it on, it is only marginally more expensive to just create a LocalVector to then later move it to a Vector for passing on by using operator Vector<T>.

@hpvb
Copy link
Member

hpvb commented Dec 21, 2024

I have changed my mind a little bit, I think that what I wrote above is true, it is also kind of off topic.

When refcount == 0, the only valid state for ptr() is nullptr. Anything else would be a violation of the contract.

EDIT: that is to say you are right. :)

core/templates/cowdata.h Outdated Show resolved Hide resolved
@Ivorforce Ivorforce force-pushed the cowdata-destruct-graciously branch from 8e8c214 to e644048 Compare December 21, 2024 18:18
Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for a weird corner case, will likely fix some random crashes.

@hpvb hpvb added the bug label Dec 21, 2024
@hpvb hpvb modified the milestones: 4.x, 4.4 Dec 21, 2024
core/templates/cowdata.h Outdated Show resolved Hide resolved
…a half-destructed buffer. This can avoid problems if any of the destructed objects tries to access the data while it's being destructed.
@Ivorforce Ivorforce force-pushed the cowdata-destruct-graciously branch from e644048 to 25cd923 Compare December 21, 2024 19:02
Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving because of changes.

@Repiteo Repiteo merged commit 08d4dd7 into godotengine:master Dec 23, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 23, 2024

Thanks!

@Ivorforce Ivorforce deleted the cowdata-destruct-graciously branch December 23, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants